Added check to handle token parsing with claims without "sub". #3287
Conversation
wassafshahzad
force-pushed
the
fix/claim-parsing-oidc-introspection
branch
from
03721d1 to
c7467c4
Compare
|
Can you please add a test case? |
Could you give me some pointers, I have been looking into multiple filter tests and oidc_intorspection_tests but cant wrap my head around on how to test it. |
|
hi @wassafshahzad, thanks for your contribution! You can check skipper/filters/auth/oidc_test.go Line 224 in 8e6c365 & This server creates the token with those claims, I would try to find a way to override those claims and create another testcase where I create the server with the override and this case we expect the fail before the fix and succeed after. The test case should fail or succeed after triggering |
Thank you and on it |
…e token subjact if sub is not present in claims Signed-off-by: wassafshahzad <[email protected]>
Signed-off-by: wassafshahzad <[email protected]>
wassafshahzad
force-pushed
the
fix/claim-parsing-oidc-introspection
branch
from
2f71fb9 to
3cfb9fa
Compare
|
@MustafaSaber Sorry for the delay, I had a bit of a tough time resolving this issue. I tried using However I stumbled on to this piece of code from the jwt_validation file and decided to use it since we already have it for validation and thought consistency would be the right way to go. |
|
Thanks for the update! We will review when we have time, we can't merge in the next couple of days also. |
No issue, Can I pick another issue ? |
|
ofc feel free to pick one of the issues 😄 |
|
hi @wassafshahzad, can you rebase? |
Yeah sure no problem |
| sub := token.Claims["sub"].(string) | ||
| sub, ok := token.Claims["sub"].(string) | ||
| if !ok { | ||
| unauthorized(ctx, sub, invalidSub, "", "") |
There was a problem hiding this comment.
Here the sub will be empty? Maybe then pass empty value ("") or a sentinel value.
There was a problem hiding this comment.
I think we can also pass r.Host like in the invocations above.
| @@ -165,6 +166,17 @@ func TestOIDCQueryClaimsFilter(t *testing.T) { | |||
| expected: 200, | |||
| expectErr: false, | |||
| }, | |||
| { | |||
| msg: "secure sub/path is not permitted", | |||
There was a problem hiding this comment.
Maybe "missing sub claim is not permitted"
|
As a panic fix it looks good to me (with minor comments). |
Description
In the oidc_introspection.py
func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext)function get value form the Claim map usingsubkey and then casts the value into a string, but this would panic if no sub key is present. I added a check to ensure we only cast values if sub key is present.Linked Issue
closes #3216
Approach to solution
As describe in the following comment, I dug a bit deeper and found 2 suitable values to use if subject is not present in token. One is the Subject value in tokenContainer struct or UserInfo Struct. I decided to go with the tokenContainer struct. If this is not correct please do guide me if UserInfo Struct would be better.